Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SIV #319

Merged
merged 5 commits into from
Jan 9, 2025
Merged

Add SIV #319

merged 5 commits into from
Jan 9, 2025

Conversation

sjaeckel
Copy link
Member

@sjaeckel sjaeckel commented Oct 25, 2017

Checklist

  • documentation is added or updated
  • tests are added or updated

This is a PoC/RFC for adding the enc+auth mode RFC5297 SIV - Synthetic Initialization Vector.

Feel free to tear it apart, improvements on the API welcome.

I had to decide how to process the AD's

  1. incremental
  2. by passing a vararg
  3. by passing an array of pointers

1 wasn't really an option AFAIU the RFC
whether 2 or 3 I was like ¯\_(ツ)_/¯ so I went for 3

I didn't really look if it would make sense to have the context exposed so we could split the processing up in init()->add_ad()->{en,de}crypt()->done()

@karel-m
Copy link
Member

karel-m commented Oct 25, 2017

Does this enable implementation of AES-GCM-SIV?

https://tools.ietf.org/html/draft-irtf-cfrg-gcmsiv-06

@sjaeckel
Copy link
Member Author

sjaeckel commented Oct 25, 2017

I haven't looked at that draft yet but this PR implements the "predecessor"

@sjaeckel
Copy link
Member Author

I've read a bit through the ML and I think we should wait until the RFC is finished to prevent something like #256.

@sjaeckel sjaeckel added this to the next milestone Oct 25, 2017
@sjaeckel
Copy link
Member Author

sjaeckel commented Nov 5, 2017

Okay I played a bit with the implementation and I'm going to add an incremental add_AD() function.

Also there should be a siv_memory() function which has to support multiple AD's in one function call. I think I'll go the varargs way for that as it's already used as a pattern in the library whereas the array of pointers isn't. Any better ideas?

@karel-m
Copy link
Member

karel-m commented Nov 5, 2017

Just FYI - there is a bunch of AES-GCM-SIV test vectors in wycheproof test suite (look for aes_gcm_siv_test.json).

@sjaeckel
Copy link
Member Author

sjaeckel commented Nov 6, 2017

ust FYI - there is a bunch of AES-GCM-SIV test vectors in wycheproof test suite

thanks, but they don't help me now as this is only AES-SIV :)

I found those:
https://github.com/randombit/botan/blob/master/src/tests/data/aead/siv.vec
https://github.com/cryptomator/siv-mode/blob/master/src/test/resources/testcases.txt (attention, the file has 17mb ... you probably shouldn't click on the link ;) )

and I planned to hand-pick some of the cryptomator/siv-mode

@karel-m
Copy link
Member

karel-m commented Apr 10, 2021

Now exists as RFC8452 (April 2019):

@levitte
Copy link
Collaborator

levitte commented Aug 20, 2024

Also there should be a siv_memory() function which has to support multiple AD's in one function call. I think I'll go the varargs way for that as it's already used as a pattern in the library whereas the array of pointers isn't. Any better ideas?

That would essentially be siv_memory_multi() no?

This PR looks interesting, but needs some love... or is it abandoned?

@sjaeckel
Copy link
Member Author

This PR looks interesting, but needs some love... or is it abandoned?

Absolutely not abandoned, it just needs some love.

@sjaeckel
Copy link
Member Author

My idea was to refactor what exists into a similar API to what we usually provide as XYZ_memory_multi(), maybe in the style of crypt_fsa().

I would not split it up into an iterative/incremental API.

What do you think? Do you have a better idea?

@levitte
Copy link
Collaborator

levitte commented Dec 15, 2024

I'd like to see a variant that takes an array of pointers as well, for convenience for programs that build the set of ADs dynamically.

@sjaeckel
Copy link
Member Author

I'd like to see a variant that takes an array of pointers as well, for convenience for programs that build the set of ADs dynamically.

The current API would satisfy that, right?

It feels a bit unnatural/clumsy to use IMO, but I also don't see a better way to provide an API that can accomplish this.

Do we even need another version of the API?

@levitte
Copy link
Collaborator

levitte commented Dec 16, 2024

Yes. Sorry, what I meant, I'd rather not lose that API if a siv_memory_multi() comes along.

Signed-off-by: Steffen Jaeckel <[email protected]>
Signed-off-by: Steffen Jaeckel <[email protected]>
@sjaeckel
Copy link
Member Author

I decided on a mix for siv_memory(): vararg for AD, the rest only gets a single pointer.

@sjaeckel sjaeckel changed the title [PoC/RFC] Add SIV Add SIV Dec 17, 2024
@sjaeckel sjaeckel requested a review from levitte December 17, 2024 17:26
@sjaeckel
Copy link
Member Author

sjaeckel commented Dec 17, 2024

Oh yeah, that looks like a fun one to debug ... seems like something makes the build of the new siv code with clang fail when -DARGTYPE > 1 🤷

It fails for ARGTYPE={2,3,4}, also with clang-18.

@sjaeckel sjaeckel force-pushed the add/SIV branch 2 times, most recently from 8602492 to dae0cd8 Compare December 18, 2024 10:35
@sjaeckel sjaeckel force-pushed the add/SIV branch 2 times, most recently from b591fea to 5822c65 Compare December 18, 2024 15:26
@sjaeckel
Copy link
Member Author

sjaeckel commented Dec 18, 2024

While reading through the RFC before writing the documentation, I'm not so sure anymore if we should really hide the detail of "how a nonce is treated in SIV".

I'm thinking of adding the nonce as a separate argument to the API, what do you think? Currently it's kind of hidden as it must be passed as the "last ad element".

Additional to that I'm not so sure if the "only one-shot API" way is the good way (resp. if it's even usable outside of the testcases)! If it's used in some kind of network protocol, some state will most likely have to be persistent/incrementing and can't always restart from scratch ... or am I missing something?

Signed-off-by: Steffen Jaeckel <[email protected]>
@levitte
Copy link
Collaborator

levitte commented Dec 20, 2024

Actually, if you read RFC 5297 carefully, you can see that it doesn't stipulate that the nonce must be the last AD input, but rather that it's "for the purpose of interoperability"... which is, of course, a good choice.

It's also perfectly possible not to have a nonce at all.

So in essence, this comes down to what choices we give the developer using libtomcrypt.
Of course, it's not wrong to have an explicit nonce argument, and if we want to allow non-interoperable encryption, allow the caller to pass NULL there.

Me, I'm leaning toward leaving the choice of how to handle the nonce, if any, to the caller. It does mean that the caller must understand how a nonce can be handled, and how it should be handled for interop... but that's a question of documentation, no?

Regarding "only one-shot API", I can't see any technical reason against having an incremental interface as well. But, must it be added in this PR, or could that be further development?

@sjaeckel sjaeckel force-pushed the add/SIV branch 2 times, most recently from 4f25fa2 to fd3df39 Compare December 28, 2024 18:02
As a new private API.

Signed-off-by: Steffen Jaeckel <[email protected]>
@sjaeckel
Copy link
Member Author

sjaeckel commented Dec 28, 2024

Me, I'm leaning toward leaving the choice of how to handle the nonce, if any, to the caller. It does mean that the caller must understand how a nonce can be handled, and how it should be handled for interop... but that's a question of documentation, no?

I've played a bit with the API and it feels just fine if the nonce has no separate API parameter.

Regarding "only one-shot API", I can't see any technical reason against having an incremental interface as well. But, must it be added in this PR, or could that be further development?

It could be further development, but I'd prefer to think of how we'd name it then before merging this PR. I guess if we decided to have an iterative API, it should have two API functions siv_encrypt() and siv_decrypt() to align with the rest of the LTC APIs.

Therefore I've renamed these APIs to siv_{en,de}crypt_memory().

* Rename `siv_{en,de}crypt()` to `siv_{en,de}crypt_memory()`.
* The number of AAD components per SIV operation must not exceed 126.
* Init OMAC only once per SIV operation.
  All OMAC operations start off with the same key. Instead of
  re-initializing the OMAC context for each operation, init once and
  store the context.
* Add SIV to timing demo.
* Add 1000-times-encrypt-then-decrypt test for SIV.
* Update docs.

Signed-off-by: Steffen Jaeckel <[email protected]>
@levitte
Copy link
Collaborator

levitte commented Jan 9, 2025

This is workable enough.

@sjaeckel sjaeckel merged commit 2e9f2b5 into develop Jan 9, 2025
75 checks passed
@sjaeckel sjaeckel deleted the add/SIV branch January 9, 2025 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants